@gabrielbosio do you have an opinion about the points I highlighted in the issue description?

@gabrielbosio do you have an opinion about the points I highlighted in the issue description?

It depends if we want to migrate the ContractAddress struct from the sequencer repo. If that's the case then the try_from method should only check for the upper bound like it's done here.

@dorimedini-starkware you are maintaining the sequencer repo?
What do you think?
Is there a reason not to have a common ContractAddress shared between the whole ecosystem (sequencer, starknet-rs, ...)

@dorimedini-starkware you are maintaining the sequencer repo? What do you think? Is there a reason not to have a common ContractAddress shared between the whole ecosystem (sequencer, starknet-rs, ...)

no reason I can think of, but this suggested implementation differs from ours in several ways, like:

1. we have another indirection (ContractAddress(pub PatriciaKey(Felt)) vs. ContractAddress(Felt))
2. we allow zero and one as valid addresses
3. we use derive_more::Display and derive_more::Deref
4. serde derives are not feature-gated

@dorimedini-starkware

1. If I remember correctly the intermediate indirection to PatriciaKey is made useless by the fact that the internal value is pub. Which mean you can freely overstep the types restriction ContractAddress(PatriciaKey(<Some totally non valid Felt value>)). So by making the internal hidden, and using From/TryFrom rather than value.0 we will increase the type safety, without need for internal nesting of types
2. This make sense. I believe we can ma them valid values, and just add a checking method is_regular_contract_address(&self) -> bool that allow for users to check if they are not receiving special address values. An alternative is to have a second type, in the same way we have NonZeroFelt, RegularContractAddress (or something). Maybe the best solution, as it is still optional, but provide the best protection for users willing to use it
3. Sure I can add those quality of life impl
4. You can just enable the feature and it will be the same. But it is a good practice to have non-core features requiring the import of depencies (like serde , num-traits, and so on) feature gated. It doesn't cost much and is a good practice.

@dorimedini-starkware

1. If I remember correctly the intermediate indirection to `PatriciaKey` is made useless by the fact that the internal value is `pub`. Which mean you can freely overstep the types restriction `ContractAddress(PatriciaKey(<Some totally non valid Felt value>))`. So by making the internal hidden, and using `From`/`TryFrom` rather than `value.0` we will increase the type safety, without need for internal nesting of types

2. This make sense. I believe we can ma them valid values, and just add a checking method `is_regular_contract_address(&self) -> bool` that allow for users to check if they are not receiving special address values. An alternative is to have a second type, in the same way we have `NonZeroFelt`, `RegularContractAddress` (or something). Maybe the best solution, as it is still optional, but provide the best protection for users willing to use it

3. Sure I can add those quality of life `impl`

4. You can just enable the feature and it will be the same. But it is a good practice to have non-core features requiring the import of depencies (like `serde` , `num-traits`, and so on) feature gated. It doesn't cost much and is a good practice.

1. PatriciaKey is pub but it's internal Felt is not, and creating a PatriciaKey with an out of range Felt should panic. I'm not saying the safety changes, I'm saying changing this indirection will require wide refactors if and when we migrate our ContractAddress type
2. .
3. please do :)
4. makes sense.

all in all I have no issue with this addition; it may take time for us to migrate to this new type though

@dorimedini-starkware so do you want this crate to also add a PatriciaKey type. Would it be helpful for you?

@dorimedini-starkware so do you want this crate to also add a PatriciaKey type. Would it be helpful for you?

I guess it would make the transition less painful, so sure.
We use PatriciaKey to describe an index in any of our state trees (only one of which is actually indexed by contract addresses) - the class tree and each storage tree also need to be bound by the patricia key limit

@dorimedini-starkware going back to point 2.

using derive_more::Display, or manual impl end up being the same. So no worries here

Now regarding derive_more::Deref, we won't implement it and will ask you to use as_ref or into instead.
Indeed the Deref trait purpose is to create smart-pointers, to add on the native ownership semantic, not to perform implicit type conversion. I understand the attraction of having the compiler quietly do those for you, but this is not what this trait is designed for and can be surprising for the user.
It is a bit more verbose, but for the best.